fix(consensus): screen never-staked slash entries, close the settlement window, unbrick recipient-facing payouts - #169
Merged
Conversation
…nt window, unbrick recipient-facing payouts Three findings from the 2026-08-12 internal audit, all pre-existing. applySlashes accepted an entry for an address that never staked. `isRetired` returns false for an `Undefined` address by design, and a zero balance fails the `balance > amount` test for every amount including zero, so the entry fell through to the ejection branch. An address that never held a ConsensusNFT reverted `_burn` with ERC721NonexistentToken, and since applySlashes runs in the closing block that revert means the block cannot be produced. An address that was whitelisted but never staked burned successfully and shipped a full genesis stake amount to Issuance, drawn from the collateral backing every other validator, with no balance of its own to draw on. The system caller does no membership filtering, so both are screened by status here. applyIncentives needs no matching guard: capping weight at the outstanding balance already leaves a zero-balance entry weightless. Screening by status there measured 432 gas per rewardee on the boundary path, so the reason it is safe is recorded in a comment rather than paid for twice. `mint` now stamps the address onto the still-`Undefined` record. Burning a whitelisted validator before it ever stakes ran `_retire` against a record whose `validatorAddress` was zero, so ValidatorRetired named an address no log-based monitor could attribute. The immediate `Staked` lane of requestStakeVersionChange debited the balance and pushed the surplus before recording the new stake version. Inside that window the validator read as slashed by exactly the surplus, and a recipient reentering topUpSlashedStake could restore stake it never lost and book the difference as rewards applyIncentives never issued. EIP-7702 is what makes this reachable: the recipient of an undelegated validator is the validator address itself, which since Prague can carry a delegation designator and execute code on receiving value. The version write is hoisted above the settlement, and topUpSlashedStake and activate are additionally nonReentrant. The unstake payout pushed to the recipient with full gas and no fallback. For a delegated validator that recipient is the delegator, an address the validator can neither change nor remove, and a delegator that was a plain EOA when the delegation was formed can attach a reverting 7702 handler afterwards. That stranded the stake permanently, leaving governance only `burn`, which confiscates rather than returns. The payout now pushes with the same bounded stipend the escrow returns use and falls back to a pull-based credit. The credit splits along its two funding sources, stake on the registry and rewards on Issuance, and claimRefund pays both in the single transfer the push would have made, capping the reward leg at what Issuance holds so a dry reward pool defers that leg rather than blocking the stake leg. An uncoverable reward leg still reverts unstake outright, so acceptRewardShortfall remains the only way to settle for less than the full amount. A review pass over the above turned up two more, both fixed here. The immediate `Staked` stake-decrease settlement was the last recipient-facing push in a user operation with no fallback, so the same hostile delegator that could no longer strand a withdrawal could still block a version change; it now routes through `_settleValue`, matching what the boundary lane does for the identical settlement. And claimRefund could complete as a transfer of zero when the only credit left was a reward leg and Issuance had run dry, reporting a claim that delivered nothing; it now reverts instead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes three findings from the 2026-08-12 internal audit, plus two more that a review pass over those fixes turned up. All five are pre-existing; none were introduced by #165.
1.
applySlashescan halt the chain or drain collateral on an unregistered entry (high, latent)applySlashesaccepted a slash entry naming an address that never staked. Nothing on the path caught it.isRetiredreturns false for anUndefinedaddress by design (it never existed, so there is nothing to have retired), and a zero balance fails thebalance > amounttest for every amount including zero, so the entry fell through to_consensusBurn:_burnwithERC721NonexistentToken.applySlashesruns in the closing block, so that revert means the block cannot be produced.balances[], at which point the Issuance consolidation insideconcludeEpochreverts and halts the chain anyway.Both reproduce against the unfixed code, as
ERC721NonexistentTokenand as a silent balance drain respectively. The entries are now screened by status.This is latent today because slashing is disabled for the MNO pilot and no slash producer exists in the node (
epoch_boundary_slashes()is a hardcoded empty vec). It is a gate on enabling slashing rather than an active risk.applySlashes([])is nevertheless invoked unconditionally every epoch, and the caller is documented to do no membership filtering, so the contract is the right place for the check.applyIncentivesneeds no matching guard: capping weight at the outstanding balance already leaves a zero-balance entry weightless. We wrote that guard, measured it at 432 gas per rewardee on the boundary path, and backed it out; the reason it is safe is now recorded in a comment instead.mintalso now stamps the address onto the still-Undefinedrecord. Burning a whitelisted validator before it ever staked ran_retireagainst a record whosevalidatorAddresswas still zero, soValidatorRetirednamed an address no log-based monitor could attribute.2. Stake-decrease settlement pushed value before recording the version (medium)
The immediate
Stakedlane ofrequestStakeVersionChangedebited the balance and pushed the surplus before writing the new stake version. Inside that window the validator read as slashed by exactly the surplus: the balance was alreadynewStakeAmountwhile the recorded version still resolved tooldStakeAmount. A recipient reenteringtopUpSlashedStakepassed itsStakeNotSlashedguard, restored stake it never lost, and left the difference to be booked as rewardsapplyIncentivesnever issued.EIP-7702 is what makes this reachable. For an undelegated validator the recipient is the validator address itself, which before Prague was structurally incapable of reentering.
The version write is hoisted above the settlement, and
topUpSlashedStakeandactivateare additionallynonReentrant. The two guards are not redundant: with onlynonReentrantin place, the test that pins the mid-push read still fails.3. A delegator could permanently strand a validator's stake (medium)
_unstakepushed to the recipient with full gas and no fallback. For a delegated validator that recipient is the delegator, an address the validator can neither change nor remove (delegationsclears only inside_burnConsensusNFT). A delegator that was a plain EOA when the delegation was formed can attach a reverting EIP-7702 handler afterwards, bricking the withdrawal permanently and leaving governance onlyburn, which confiscates the stake rather than returning it.acceptRewardShortfalldoes not help: it caps the rewards leg and never bypasses the push.The payout now uses the same bounded stipend as the escrow returns and falls back to a pull-based credit. The credit splits along its two funding sources, stake on the registry and rewards on Issuance, so there is a new
claimableRewardsmapping alongsideclaimableRefunds, andclaimRefundpays both in the single transfer the push would have made.Two more from reviewing the above
Stakedstake-decrease settlement was the last recipient-facing push in a user operation with no fallback. The same hostile delegator that could no longer strand a withdrawal could still block a version change through it. It now routes through_settleValue, so both settlement lanes for that operation behave identically on a recipient that stops accepting value.claimRefundcould complete as a transfer of zero when the only credit left was a reward leg and Issuance had run dry, emittingRefundClaimed(x, 0)for a claim that delivered nothing. It reverts now.Notes for review
claimableRewardsis appended aftertopUpAuthorityRequired, the tail of the flattened layout, so every existing slot is preserved for an in-place upgrade.acceptRewardShortfallis preserved deliberately. The credit fallback would otherwise have swallowed an underfunded-Issuance revert and silently deferred instead of reverting, which two existing tests pin against._unstakenow surfaces that case explicitly before the push, reproducing Issuance's exact revert args, so accepting the shortfall remains the only way to settle for less than the full amount.claimRefundcaps the reward leg at Issuance's balance and leaves the remainder credited. Without that, an empty pool would hold up the registry-backed stake leg too.applyIncentives356,185 for 100 rewardees,concludeEpoch33,912,995 for the 1000-entry settlement wave, matching the ceiling documented ininvariants.md.claimStakeRewardsstill reverts on a recipient that rejects value. It strands nothing: the rewards stay in the ledger and come out throughunstake, which now credits.Testing
235 consensus tests pass at the default 250-run fuzz depth, 335 across the repo. Every new test was confirmed to fail against the unfixed code first:
applySlashesvariants, reproducing the halt and the draintopUpSlashedStakeexploitinvariants.mdis updated for all five, andartifacts/ConsensusRegistry.jsonis regenerated withmake update-artifacts. The other five artifacts came out with identical runtime bytecode and only source-map churn, so we left them at master.